feat(agentic-rag): add native OpenShift ServiceAccount token auth - #264
feat(agentic-rag): add native OpenShift ServiceAccount token auth#264Wojciech-Rebisz wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe agentic RAG template adds optional OpenShift ServiceAccount token authentication, local auth dependency and image-build wiring, Helm configuration for auth settings, protected unauthenticated routes, environment examples, and deployment documentation. ChangesAgentic RAG authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPIApp
participant SATokenAuthMiddleware
participant KubernetesTokenReview
Client->>FastAPIApp: Request /chat/completions
FastAPIApp->>SATokenAuthMiddleware: Process bearer token
SATokenAuthMiddleware->>KubernetesTokenReview: Submit TokenReview
KubernetesTokenReview-->>SATokenAuthMiddleware: Return token status
SATokenAuthMiddleware-->>FastAPIApp: Allow or reject request
FastAPIApp-->>Client: Return response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/langgraph/templates/agentic_rag/Makefile`:
- Around line 110-112: Update both staging recipes in
agents/langgraph/templates/agentic_rag/Makefile: lines 110-112 and 98-100.
Remove existing ./images and ./components/auth staging directories before
copying, and register the EXIT cleanup trap before either staging command so
failed and repeated builds cannot retain stale content; preserve the existing
cleanup behavior after successful builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: cfbc7feb-1b1e-4990-b1db-5e46438f538c
⛔ Files ignored due to path filters (1)
agents/langgraph/templates/agentic_rag/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
agents/langgraph/templates/agentic_rag/Dockerfileagents/langgraph/templates/agentic_rag/Makefileagents/langgraph/templates/agentic_rag/README.mdagents/langgraph/templates/agentic_rag/main.pyagents/langgraph/templates/agentic_rag/pyproject.toml
andrewdonheiser
left a comment
andrewdonheiser
left a comment
There was a problem hiding this comment.
PR Review
Important (2)
1. Missing auth env vars in .env.example
The react_agent .env.example includes commented-out auth vars for discoverability:
# Auth (local default off; comma-separate multiple allowlist entries)
# AUTH_ENABLED=false
# AUTH_AUDIENCE=langgraph-react-agent
# AUTH_ALLOWED_SERVICEACCOUNTS=ci-testing:langgraph-react-agent-caller
The agentic_rag .env.example has none of these. Developers using make init get no hint auth exists without reading the README.
Fix: Add equivalent commented-out auth vars to .env.example.
2. Missing auth passthrough in Makefile deploy / dry-run targets
The react_agent deploy target includes ~15 lines of auth handling:
- Parsing
AUTH_ALLOWED_SERVICEACCOUNTSinto proper Helm array indices (--set-string auth.allowedServiceAccounts[N]=...) - Passing
AUTH_ENABLED,serviceAccount.create, andAUTH_AUDIENCEas Helm flags
The agentic_rag deploy and dry-run targets have none of this. Setting AUTH_ENABLED=true in .env and running make deploy silently ignores all auth config. The README works around this by documenting a separate helm upgrade --reuse-values command, but this is a significant gap vs. the react_agent pattern the PR claims to match.
Fix: Port the auth allowlist parsing block and AUTH_ENABLED / AUTH_AUDIENCE / serviceAccount.create flags from react_agent's deploy and dry-run targets. [Makefile:126-159]
Suggestions (1)
README uses --set string syntax for an array-typed Helm value
The README suggests:
--set auth.allowedServiceAccounts="<namespace>:my-caller"
But values.yaml defines allowedServiceAccounts: [] (an array), and the template uses join "," .Values.auth.allowedServiceAccounts. Setting it as a string happens to work for a single SA due to Sprig's lenient join, but breaks for multiple SAs. The react_agent Makefile uses the correct array index syntax: --set-string auth.allowedServiceAccounts[0]=....
Fix: Use --set-string "auth.allowedServiceAccounts[0]=<namespace>:my-caller" in the README.
Strengths
- main.py auth code is byte-for-byte identical to react_agent — zero pattern drift
- Makefile build targets have better cleanup ordering than react_agent (rm before copy + early trap)
- Comprehensive README with step-by-step enable/disable flows and Helm values table
- Defense-in-depth: auth gated at main.py level and inside middleware
- Clean pyproject.toml with
[auth]optional dep and[tool.uv.sources]for local dev
Signed-off-by: Wojciech Rebisz <147821486+Wojciech-Rebisz@users.noreply.github.com>
Signed-off-by: Wojciech Rebisz <147821486+Wojciech-Rebisz@users.noreply.github.com>
… .env.example Signed-off-by: Wojciech Rebisz <147821486+Wojciech-Rebisz@users.noreply.github.com>
21e01dd to
2c7fdcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/langgraph/templates/agentic_rag/Makefile`:
- Around line 158-162: Update the Makefile target containing the Helm commands
to add preflight validation before both commands: when AUTH_ENABLED is true,
require AUTH_AUDIENCE to be non-empty and fail with a clear error otherwise.
Preserve the existing Helm argument construction and ensure the validation runs
before either deployment path executes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 83f33e17-f4c4-486c-a756-649a862b74ec
⛔ Files ignored due to path filters (1)
agents/langgraph/templates/agentic_rag/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
agents/langgraph/templates/agentic_rag/.env.exampleagents/langgraph/templates/agentic_rag/Dockerfileagents/langgraph/templates/agentic_rag/Makefileagents/langgraph/templates/agentic_rag/README.mdagents/langgraph/templates/agentic_rag/main.pyagents/langgraph/templates/agentic_rag/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- agents/langgraph/templates/agentic_rag/.env.example
- agents/langgraph/templates/agentic_rag/pyproject.toml
- agents/langgraph/templates/agentic_rag/Dockerfile
- agents/langgraph/templates/agentic_rag/main.py
- agents/langgraph/templates/agentic_rag/README.md
| $${AUTH_ENABLED:+--set "auth.enabled=$${AUTH_ENABLED}"} \ | ||
| $${AUTH_ENABLED:+--set "serviceAccount.create=$${AUTH_ENABLED}"} \ | ||
| $${AUTH_ENABLED:+--set "auth.createAuthDelegatorBinding=$${AUTH_ENABLED}"} \ | ||
| $${AUTH_AUDIENCE:+--set-string "auth.audience=$${AUTH_AUDIENCE}"} \ | ||
| $${AUTH_ALLOWLIST_ARGS} \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail fast when auth is enabled without an audience.
AUTH_ENABLED=true currently enables the chart even when AUTH_AUDIENCE is empty, because the audience value is silently omitted. This violates the middleware contract and can leave protected routes unusable. Add the same preflight validation before both Helm commands.
Suggested validation
+ if [ "$${AUTH_ENABLED:-}" = "true" ] && [ -z "$${AUTH_AUDIENCE:-}" ]; then \
+ echo "ERROR: AUTH_AUDIENCE is required when AUTH_ENABLED=true" >&2; exit 1; \
+ fi && \As per path instructions: “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
Also applies to: 205-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/langgraph/templates/agentic_rag/Makefile` around lines 158 - 162,
Update the Makefile target containing the Helm commands to add preflight
validation before both commands: when AUTH_ENABLED is true, require
AUTH_AUDIENCE to be non-empty and fail with a clear error otherwise. Preserve
the existing Helm argument construction and ensure the validation runs before
either deployment path executes.
Source: Path instructions
2 participants
Summary
matching the existing pattern in react_agent
Changes
SATokenAuthMiddlewareintegration; hide playground UI when auth is enabled[auth]optional dependency withagent-authcomponents/authpackage alongside the agentcomponents/authinto the build context for both local and OpenShift buildsagent-authand its dependenciesTest plan
uv sync --extra authinstalls successfullymake run-appworks with auth disabled (backward compatibility)make build-openshift+make deploysucceeds/chat/completions/chat/completions/healthaccessible without token